SSL_Handshake: close nio channel when NioClient fail to handshake wit… #10153
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10153 +/- ##
=========================================
Coverage 16.07% 16.07%
- Complexity 12885 12886 +1
=========================================
Files 5642 5642
Lines 494039 494040 +1
Branches 59912 59912
=========================================
+ Hits 79408 79414 +6
+ Misses 405828 405822 -6
- Partials 8803 8804 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
shwstppr
left a comment
shwstppr
left a comment
There was a problem hiding this comment.
Had to do something similar here, https://github.com/apache/cloudstack/pull/9840/files#diff-a96656b544a2f0b6dd8c2ed1b2232a1b8023a22025e0bf47232fe81b4be64b96
| // remaining task done | ||
| task = _factory.create(Task.Type.CONNECT, link, null); | ||
| } catch (final GeneralSecurityException e) { | ||
| _selector.close(); |
|
thanks for your contribution @bulleting0724 , can you have a quick look at the code @shwstppr is pointing at. He uses an extra null-check and |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses an SSL handshake issue whereby the TCP connection remained open, causing a server thread to hang during a failed handshake.
- Actively closes the client connection when an IOException occurs during handshake initialization
- Ensures the selector is closed following the connection close
| _selector.close(); | ||
| throw new IOException("Failed to initialise security", e); | ||
| } catch (final IOException e) { | ||
| _clientConnection.close(); |
There was a problem hiding this comment.
Consider wrapping _clientConnection.close() in a try-catch block to ensure that any exception thrown during the close operation does not obscure the original IOException and to better manage resource cleanup.
| _clientConnection.close(); | |
| try { | |
| _clientConnection.close(); | |
| } catch (final IOException closeException) { | |
| logger.error("Failed to close _clientConnection", closeException); | |
| } |
as that is merged @shwstppr , is this PR still valid? |
|
@DaanHoogland cc @bulleting0724 I think this was covered in #9840 and is present in main branch now |
|
closing @bulleting0724 , please reopen or create a new PR if you feel there is still an issue. |
github-project-automation
bot
moved this from In Progress
to Done
in Apache CloudStack 4.22.0
5 participants
Description
This PR solved an issue when cs-agent throws an exception during ssl handshake; the TCP connection is not closed between cs-server and cs-agent, which further causes the server thread to hang forever.
When the ssl handshake is at the client key exchange phrase, the server will be waiting for the agent to provide cipher suit, but at the agent side the exception could happen when the agent can’t conform to the cipher suite that the server provides, so the agent couldn’t communicate client key to the server. Thus at the server side the handshake thread is forever pending on a function that expects to read packets from SocketChannel.
Steps to reproduce this issue
1.server uses a 1024 bit rsa public key which you can verify by typing “keytool -list -storepass $keystore_password -keystore $keystore_file -v”.
2.find “Subject Public Key Algorithm” in the step 1 output.
2.at the agent, edit “JAVA_HOME/jre/lib/security/java.security” and append “RSA keySize < 2048” to jdk.tls.disabledAlgorithms.
3.restart cloudstack-agent.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
How Has This Been Tested?
Before I applied this change, in that situation and at the agent side, the state of the tcp connection was CLOSE_WAIT and remained there forever. When I applied this change, the agent actively closed the channel which in turn actively closed the tcp connection and the state of the tcp connection will move to TIME_WAIT which is a normal state indicating the connection is closing.